Use curl instead of oc binary copy for testing kubeconfig#30927
Use curl instead of oc binary copy for testing kubeconfig#30927dgoodwin wants to merge 1 commit intoopenshift:mainfrom
Conversation
The latter is causing problems on multi-arch and rhcos mismatches
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
bot
added
the
approved
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-rhcos10-techpreview |
|
@dgoodwin: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b4086a50-27aa-11f1-999d-88120be1b154-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/apiserver/kubeconfigs.go`:
- Around line 121-125: The shell snippet that builds and runs the curl check
using kubeconfigPath should be hardened: change curl flags from -sf to -Ssf so
curl prints error details to stderr, and add defensive validation after
extracting server, cert, key, and ca (the variables produced by the grep/head
pipelines that reference kubeconfigPath) to ensure each field is present and
appears exactly once (e.g., check non-empty and unique counts) and fail with a
clear error message if validation fails; update the formatted command string
surrounding kubeconfigPath accordingly so failures are visible and mis-parsed
kubeconfigs cause immediate, informative test failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28a15c7e-251d-4451-a7e1-55c0769a6869
📒 Files selected for processing (1)
test/extended/apiserver/kubeconfigs.go
| fmt.Sprintf(`server=$(grep 'server:' "%[1]s" | head -1 | awk '{print $2}') | ||
| cert=$(grep 'client-certificate:' "%[1]s" | head -1 | awk '{print $2}') | ||
| key=$(grep 'client-key:' "%[1]s" | head -1 | awk '{print $2}') | ||
| ca=$(grep 'certificate-authority:' "%[1]s" | head -1 | awk '{print $2}') | ||
| curl -sf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test/extended/apiserver && wc -l kubeconfigs.goRepository: openshift/origin
Length of output: 78
🏁 Script executed:
cd test/extended/apiserver && sed -n '110,135p' kubeconfigs.goRepository: openshift/origin
Length of output: 1251
🏁 Script executed:
cd test/extended/apiserver && sed -n '115,130p' kubeconfigs.go | cat -nRepository: openshift/origin
Length of output: 1217
🏁 Script executed:
cd test/extended/apiserver && grep -B 5 -A 5 "kubeApiserverLocations" kubeconfigs.go | head -30Repository: openshift/origin
Length of output: 1707
🏁 Script executed:
rg "verifyKubeconfig|verifyKubeapiserverKubeconfig" test/extended/apiserver/ -B 2 -A 2Repository: openshift/origin
Length of output: 42
🌐 Web query:
curl -sf vs curl -Ssf flags difference stderr error output
💡 Result:
The difference between curl -sf and curl -Ssf (or curl -sSf) lies in how they handle stderr output for errors when running in silent mode. - curl -sf (silent + fail): Suppresses the progress meter and all error messages (no output to stderr on failure). On HTTP errors (e.g., 4xx/5xx), it fails with exit code 22 but produces no output at all, including no error message on stderr. - curl -Ssf (show-error + silent + fail): Suppresses the progress meter but shows an error message on stderr if the transfer fails (e.g., connection errors or HTTP errors due to -f). This is the common combination for scripts: quiet on success, visible error on stderr on failure for logging/monitoring (e.g., cron jobs). All diagnostic output (progress, verbose, errors) from curl goes to stderr by default. Response body goes to stdout. Examples: - Success (both): No stderr output. - HTTP 404 with -sf: Exit 22, empty stderr. - HTTP 404 with -Ssf: Exit 22, error like "curl: (22) The requested URL returned error: 404" on stderr. This behavior is consistent across recent curl versions (e.g., 8.x as of 2026).
Citations:
- 1: https://unix.stackexchange.com/questions/480510/what-does-curl-ss-means
- 2: https://curl.se/docs/manpage.html
- 3: https://linuxman7.org/linux/man-pages/man1/curl.1.html
- 4: https://stackoverflow.com/questions/6935006/making-curl-send-errors-to-stderr-and-everything-else-to-stdout
- 5: https://www.man7.org/linux/man-pages/man1/curl.1.html
- 6: https://man.archlinux.org/man/curl.1.en
- 7: https://curl.se/docs/manpage.html?category=services
- 8: https://www.unix.com/man_page/mojave/1/curl/
- 9: https://stackoverflow.com/questions/7373752/how-do-i-get-curl-to-not-show-the-progress-bar
Improve test diagnostics by using curl -Ssf and add defensive validation for kubeconfig parsing.
The shell script extracts cert paths with grep | head -1 without validating that each field appears exactly once. While the test kubeconfigs are single-purpose, defensive validation prevents accidental mismatches. More importantly, curl -sf suppresses error output entirely, making failures opaque; switch to curl -Ssf to show errors to stderr for better test diagnostics.
Proposed hardening patch
- fmt.Sprintf(`server=$(grep 'server:' "%[1]s" | head -1 | awk '{print $2}')
-cert=$(grep 'client-certificate:' "%[1]s" | head -1 | awk '{print $2}')
-key=$(grep 'client-key:' "%[1]s" | head -1 | awk '{print $2}')
-ca=$(grep 'certificate-authority:' "%[1]s" | head -1 | awk '{print $2}')
-curl -sf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output()
+ fmt.Sprintf(`cfg="%[1]s"
+for k in server client-certificate client-key certificate-authority; do
+ c=$(grep -Ec "^[[:space:]]*${k}:[[:space:]]+" "$cfg")
+ [ "$c" -eq 1 ] || { echo "expected exactly one ${k} in $cfg, got $c" >&2; exit 1; }
+done
+server=$(grep -m1 -E "^[[:space:]]*server:[[:space:]]+" "$cfg" | awk '{print $2}')
+cert=$(grep -m1 -E "^[[:space:]]*client-certificate:[[:space:]]+" "$cfg" | awk '{print $2}')
+key=$(grep -m1 -E "^[[:space:]]*client-key:[[:space:]]+" "$cfg" | awk '{print $2}')
+ca=$(grep -m1 -E "^[[:space:]]*certificate-authority:[[:space:]]+" "$cfg" | awk '{print $2}')
+curl -Ssf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Sprintf(`server=$(grep 'server:' "%[1]s" | head -1 | awk '{print $2}') | |
| cert=$(grep 'client-certificate:' "%[1]s" | head -1 | awk '{print $2}') | |
| key=$(grep 'client-key:' "%[1]s" | head -1 | awk '{print $2}') | |
| ca=$(grep 'certificate-authority:' "%[1]s" | head -1 | awk '{print $2}') | |
| curl -sf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output() | |
| fmt.Sprintf(`cfg="%[1]s" | |
| for k in server client-certificate client-key certificate-authority; do | |
| c=$(grep -Ec "^[[:space:]]*${k}:[[:space:]]+" "$cfg") | |
| [ "$c" -eq 1 ] || { echo "expected exactly one ${k} in $cfg, got $c" >&2; exit 1; } | |
| done | |
| server=$(grep -m1 -E "^[[:space:]]*server:[[:space:]]+" "$cfg" | awk '{print $2}') | |
| cert=$(grep -m1 -E "^[[:space:]]*client-certificate:[[:space:]]+" "$cfg" | awk '{print $2}') | |
| key=$(grep -m1 -E "^[[:space:]]*client-key:[[:space:]]+" "$cfg" | awk '{print $2}') | |
| ca=$(grep -m1 -E "^[[:space:]]*certificate-authority:[[:space:]]+" "$cfg" | awk '{print $2}') | |
| curl -Ssf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/apiserver/kubeconfigs.go` around lines 121 - 125, The shell
snippet that builds and runs the curl check using kubeconfigPath should be
hardened: change curl flags from -sf to -Ssf so curl prints error details to
stderr, and add defensive validation after extracting server, cert, key, and ca
(the variables produced by the grep/head pipelines that reference
kubeconfigPath) to ensure each field is present and appears exactly once (e.g.,
check non-empty and unique counts) and fail with a clear error message if
validation fails; update the formatted command string surrounding kubeconfigPath
accordingly so failures are visible and mis-parsed kubeconfigs cause immediate,
informative test failures.
|
Scheduling required tests: |
|
@dgoodwin: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, prb112 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
The latter is causing problems on multi-arch and RHCOS 10.
This is quite ugly and a better version might be a good idea, but it did appear to work in the payload job run below. Coderabbit comment looks worth addressing, but we might want a cleaner version of this anyhow.
Hoping someone can pick this up and run it down to solve the two related bugs.